-
-
Notifications
You must be signed in to change notification settings - Fork 44.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes The Bug In Radix Tree. Issue #11316 #11385
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
Based on ruff Recommendations - Removed WhiteSpaces - Moved the Import to beginning - Replaced assertTrue with regular assert
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Hey @spazm. Can you please review this PR. Radix Tree had a bug. And Excuse me for the commit mess. Ruff gave me some problems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
@@ -1,3 +1,5 @@ | |||
import unittest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We (and the majority of the Python community) use pytest instead of unittest so all this boilerplate is not needed.
As discussed in CONTRIBUTING.md, changing both code and tests in a single pull request makes code review quite difficult.
Can you please create a separate simple pull request that adds a test that proves that word = ""
fails?
Once that is proven, we can determine how to proceed with this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails on subseunt recursive calls not on direct addition of "". Please check the issue it is trying to fix. I have added the same word insertions in Unit Test as well
|
||
class TestRadixNode(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This boilerplate adds unnecessary complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure Will Remove it. But its the best way to do this
Describe your change:
Fixes #11316 .
There is a bug in the code which doesn't handle the edge case when the insertion of empty string is done.
Checklist: